-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[skip ci] Use new platform CLI as the main entry point #19994
Conversation
10df503
to
f5b304e
Compare
e3f5cb8
to
9821d83
Compare
Hey @elastic/kibana-platform, Would you mind giving this PR a spin? There is no need to do a code review (unless you really want to give preliminary feedback!) as I'll split it into more reviewable chunks/PRs later on. Also there are no recommendations on how to test this one since I'm really interested in the way you use CLI on day to day basis so that we can notice issues I've missed. Please, see Known issues or changes in behaviour and let me know what you think should be handled or improved on the very first iteration. There are likely some other subtle differences I haven't mentioned, so feel free to comment about any suspicious CLI behavior. Thanks! |
Okay, here's quick update on the issue above:
I've updated first point in |
The CLI commands are consistent across the stack, so we don't want to change them unless there's a good reason and we coordinated that with the ingest and elasticsearch teams. |
I do think it's important that we don't log the host and port until all endpoints are actually loaded up. It's nice that the base path proxy handles it for you, but I'm far more interested in the behaviors when the basepath proxy is not present than when it is. I just tested on a snapshot build of this branch and it took multiple seconds between when we logged the statement and when Kibana would stop spitting out errors. This doesn't appear to have anything to do with optimization because it's happening on a fresh install from a snapshot, so the optimizer is not firing. Perhaps we could just pass a function with the binding logic into legacy server that it can invoke when it's all loaded up? |
I got you, but it seems it doesn't stop us from using
Yeah, optimizer is just the slowest thing, but we also have a bunch of other initialization logic in the legacy world.
We don't need to pass anything since core now manages |
I'm not really sure what you mean by this, so let's just sync up whenever we do tackle that.
Awesome! |
I tested running Kibana with a snapshot build as well as installing a plugin with a snapshot build, and other than the feedback I had about the timing of the hostname/port log, it seemed good. I'm sure I'll have more concrete feedback on a PR by PR basis. |
This comment has been minimized.
This comment has been minimized.
c28ac07
to
bccd53f
Compare
bccd53f
to
a5989d4
Compare
As it turned out we can make /cc @elastic/kibana-operations, just in case you may find something useful in this PR at some point since, if I understand correctly, CLI is on your plate. |
This PR serves as a sandbox for all the work that is being done for #19324, once it's ready some tasks may end up in separate PRs.
Tasks:
new-platform
branch into mastercore
and "legacy" Kibanacommander
), butcore
CLI (viayargs
) is still missing (e.g.oss
,repl
etc.)kbnServer.newPlatform
object tokbnServer.core
duplex broadcast channel (viaEventEmitter
) to make communication betweencore/legacy_compat
and "legacy" KIbana easier (experimental, will need feedback)LegacyService
that will managekbnServer
life-cyclecore
should be able to merge CLI arguments with config values like we do in mastercore
should be able to merge keystore content with config values like we do in mastercore
CLI withClusterManager
and get rid of hacks we had to introduce inBasePathProxy
previously to overcome circular dependency betweenBasePathProxy
andClusterManager
CAN_CLUSTER
,XPACK_OPTIONAL
etc.)core
should respectserver.autoListen = false
used by theoptimizer
workercore
CLI withrepl
core
should correctly processSIGINT
/SIGTERM
/SIGHUP
and propagate that to the "legacy" Kibana when needed (e.g.applyLoggingConfiguration
onSIGHUP
)core
CLI (viayargs
)--help
output look similar as much as possible to what "legacy" CLI produces (viacommander
)src/legacy/cli
(e.g.ClusterManager
andrepl
)Known issues or changes in behaviour:
When Kibana is run for the first time in non-dev mode, in console/terminal we'll see URL that should be used to open Kibana almost immediately (logged by the core HTTP Server), but it won't accessible until optimization phase in the "legacy" Kibana is completed. Previously we displayed that message only when Kibana is entirely ready (inkbnServer.listen
). I'm not sure whether it's something we should really care about, but it still may confuse some people or maybe there is some automation around that "Kibana is ready" string too?When Kibana is accessed during optimization phase in "legacy" world user would get
Unable to connect
since Kibana isn't listening on the port yet, in "new platform" world we already have listener before optimization phase is started so we respond with503
andRetry-After
header (andKibana server is not ready yet
body) until Kibana is fully initialized. If Kibana is accessed through base path proxy then the proxy will "pause" request until Kibana is ready, and503
will be seen only if user tries to bypass base path proxy (going directly to:5603
).Yargs is responsible for help/usage rendering now, so it not only displays argument's type and "mandatoriness", but default value as well. It's good in general, but looks a bit ugly for long default values (like paths to configs). Currently I rely on yargs default value functionality for config/plugins paths and
--help
output may look a bit broken, I can get rid of it if you feel it looks really bad.I removed support for dedicated
help
command in favor of generic--help
argument (./bin/kibana help serve
vs./bin/kibana serve --help
), that's supported out of the box and feels much more natural to me, but would like to hear what you think is better.cli_keystore
andcli_plugin
still rely onsrc/legacy/cli/command.js
, I didn't touch them so we'll have inconsistent CLI behavior for main and "secondary" CLI's for now (I'd probably convert them to main CLI sub-commands e.g../bin/kibana keystore create
).If CLI fails I display
Specify --help for available options
message right after error message instead of full--help
content like is done in "legacy" Kibana. Don't really know why, probably I just fed up with it, seen it so many times while working on this PR :) Tell me if you prefer to have it, and I'll return it back.The "legacy" serve command rejects unknown single hyphen options as extra options that will be merged with the main config, I didn't include this piece of code in CLI and deferred that to config validation that will fail with unused config values anyway. Tell me if you prefer the previous behavior.
Blocked by:
Introduceschema.any
(PR, merged)Make core logging independent from the legacy Kibana (PR, merged)MergeMutableLoggerFactory
andLoggingService
(PR, in review)Revamp core environment class to support upcoming core<-->legacy bootstrap inversion (PR, in review)core
responsible for reading and merging of config files. Simplify legacy config adapter. (PR, ready for review)LegacyService
. Usecore
to start legacy Kibana. (PR, ready for review)